Retry tests failing on cuBLAS allocation errors the rerun filter missed - #6916
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
660aefe to
8ef423b
Compare
8ef423b to
7ce6dae
Compare
albertvillanova
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7ce6dae. Configure here.
| # Transient infrastructure errors that are worth retrying, matched against "<ExceptionType>: <message>": | ||
| # - OSError, Timeout, HTTPError 502/504: Hub flakiness | ||
| # - OutOfMemoryError, STATUS_ALLOC_FAILED: GPU memory pressure from the parallel workers | ||
| rerun_errors := (OSError|Timeout|HTTPError.*502|HTTPError.*504|OutOfMemoryError|STATUS_ALLOC_FAILED) |
There was a problem hiding this comment.
Incomplete GPU retry patterns
Medium Severity
rerun_errors adds STATUS_ALLOC_FAILED but still omits the torch.testing.assert_close wrapper text. OOMs raised inside assert_close surface as RuntimeError: Comparing... with OutOfMemoryError only on __cause__, which --only-rerun never inspects, so that failure shape still skips retries and can fail a healthy branch under GPU memory pressure.
Reviewed by Cursor Bugbot for commit 7ce6dae. Configure here.
There was a problem hiding this comment.
The analysis is accurate, but this is a deliberate scope boundary rather than an oversight, and adding the wrapper text would make the filter worse rather than better.
The gap is real and described in the PR body under "Not addressed here", and it is the half of #6917 that is intentionally left open. The reason it is not closed here is that the only available handle is the wrapper text itself, and that text says nothing about memory pressure:
raise RuntimeError(
f"Comparing\n\n"
f"{pair}\n\n"
f"resulted in the unexpected exception above. "
...
) from errorThe message carries only the tensor pair repr and boilerplate. torch.testing emits it for any exception it does not expect during a comparison, so matching it would also retry genuine defects, a dtype bug surfacing as a TypeError, an unsupported layout raising NotImplementedError, and so on. A deterministic bug would still fail all five attempts, so it would not be hidden, but a nondeterministic one would be silently retried into green, which is precisely what a narrow --only-rerun exists to prevent. Trading a false negative on OOM for a false positive on real bugs is the wrong direction.
An earlier revision of this PR did include that pattern and it was removed for this reason.
I verified the resulting behaviour against synthetic failures for each shape. Retried: a directly raised OutOfMemoryError, and CUBLAS_STATUS_ALLOC_FAILED. Not retried: an OOM wrapped by assert_close, a plain failing assertion, an ordinary assert_close value mismatch, and a TypeError raised inside a comparison.
The correct place to fix the remaining shape is pytest-rerunfailures, which matches only the outermost exception:
def _try_match_error(rerun_errors, excinfo):
if excinfo:
err = f"{excinfo.type.__name__}: {excinfo.value}"
for rerun_regex in rerun_errors:
if re.search(rerun_regex, err):
return True
return FalseIf that walked __cause__ and __context__, the existing OutOfMemoryError pattern would match the wrapped case on its own, with no ambiguity and no need for a wrapper-text heuristic. #6917 stays open to track that.
qgallouedec
left a comment
qgallouedec
left a comment
There was a problem hiding this comment.
lgtm.
Good call not matching the assert_close wrapper text, that would retry genuine failures. The rerun_errors variable is better, that regex was unreadable.
2 participants
This PR makes the CI rerun filter retry cuBLAS allocation failures, which it was silently letting through even though they are pure GPU memory pressure on the shared runner.
Partially addresses #6917.
Motivation
pytest-rerunfailuresmatches--only-rerunagainst the outermost exception only, building the string it tests asf"{excinfo.type.__name__}: {excinfo.value}". The chained__cause__is never consulted.A cuBLAS handle failing to allocate under memory pressure therefore escaped the filter, since it arrives as a plain
RuntimeErrorthat no pattern matched:This hit the dev-dependencies job in https://github.com/huggingface/trl/actions/runs/32855030464/job/97824800816, on a GPU with 16 MiB free across roughly 33 concurrent workers. The affected test passed in the three sibling jobs of that run and again on a re-run, so nothing was broken, the retry that exists for exactly this case just did not fire.
Solution
Add
STATUS_ALLOC_FAILEDto the pattern list, matched rather than the cuBLAS-specific spelling so cuSOLVER and cuSPARSE allocation failures are covered too, and lift the list into arerun_errorsvariable so each entry can carry a comment explaining what it is for.Verified against synthetic failures reproducing each shape: a direct OOM and a cuBLAS allocation error are retried, while a plain failing assertion, an
assert_closevalue mismatch, and a genuineTypeErrorraised inside a comparison are all still failed immediately.Not addressed here
The second half of #6917 is left open on purpose. When an OOM is raised inside
torch.testing.assert_close,torch.testingwraps it inRuntimeError("Comparing\n\n{pair}\n\nresulted in the unexpected exception above. ..."), and that message drops the original error text, so the failure surfaces asRuntimeError: ComparingwithOutOfMemoryErrordemoted to a chained cause. Nothing in the outer message indicates memory pressure, so no regex can distinguish it from a real defect surfacing inside a comparison. Matching the wrapper text would retry genuine bugs, including nondeterministic ones that the narrow filter exists to avoid masking. The proper fix is forpytest-rerunfailuresto match against the exception chain.Changes
STATUS_ALLOC_FAILEDto the--only-rerunpattern listrerun_errorsMakefile variableNote
Low Risk
Only changes pytest retry rules in the Makefile; no library or runtime behavior is affected.
Overview
CI’s default
make testretry list is refactored into a documentedrerun_errorsMakefile variable and wired into--only-rerun, so the pattern is easier to maintain and comment.The regex now also matches
STATUS_ALLOC_FAILED, covering transient cuBLAS/cuSOLVER/cuSPARSE handle allocation failures under parallel GPU load that previously surfaced as unmatchedRuntimeErrormessages and were not retried (unlike directOutOfMemoryErrorcases that were already in the list).Reviewed by Cursor Bugbot for commit 7ce6dae. Bugbot is set up for automated code reviews on this repo. Configure here.